Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove direct use of "setup.py sdist", add targets "make SPKG-sdist" #35104

Merged
merged 13 commits into from
Mar 19, 2023
Merged

Remove direct use of "setup.py sdist", add targets "make SPKG-sdist" #35104

merged 13 commits into from
Mar 19, 2023

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Feb 13, 2023

📚 Description

Fixes #34855

📝 Checklist

  • I have made sure that the title is self-explanatory and the description concisely explains the PR.
  • I have linked an issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

@mkoeppe mkoeppe added this to the sage-10.0 milestone Feb 13, 2023
@codecov-commenter
Copy link

codecov-commenter commented Feb 13, 2023 •
edited
Loading

Codecov Report

Base: 88.58% // Head: 88.59% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (c0f6348) compared to base (293dd72).
Patch has no changes to coverable lines.

Additional details and impacted files 
@@            Coverage Diff            @@
##           develop   #35104    +/-   ##
=========================================
  Coverage    88.58%   88.59%            
=========================================
  Files         2140     2140            
  Lines       396961   397273   +312     
=========================================
+ Hits        351655   351953   +298     
- Misses       45306    45320    +14
Impacted Files Coverage Δ
src/sage/groups/affine_gps/euclidean_group.py 88.88% <0.00%> (-11.12%) ⬇️
src/sage/quadratic_forms/random_quadraticform.py 90.00% <0.00%> (-6.00%) ⬇️
src/sage/plot/histogram.py 96.77% <0.00%> (-3.23%) ⬇️
src/sage/groups/affine_gps/affine_group.py 96.62% <0.00%> (-2.25%) ⬇️
src/sage/cpython/_py2_random.py 74.79% <0.00%> (-1.24%) ⬇️
src/sage/modular/hecke/algebra.py 94.65% <0.00%> (-1.07%) ⬇️
src/sage/modular/quasimodform/element.py 99.14% <0.00%> (-0.86%) ⬇️
src/sage/modular/modform/numerical.py 94.19% <0.00%> (-0.65%) ⬇️
src/sage/combinat/colored_permutations.py 95.46% <0.00%> (-0.60%) ⬇️
src/sage/rings/polynomial/ore_polynomial_ring.py 89.40% <0.00%> (-0.43%) ⬇️
... and 41 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mkoeppe mkoeppe self-assigned this Feb 18, 2023
@github-actions
Copy link

Documentation preview for this PR is ready! 🎉
Built with commit: c0f6348

@mkoeppe mkoeppe requested a review from dimpase February 26, 2023 20:39
@jhpalmieri
Copy link
Member

On a Mac, it looks like the current version of make pypi-sdists is broken: it doesn't produce tarballs for all nine of the listed packages, creating 5 of them and then "UNKNOWN-0.0.0.tar.gz". With the changes here, it looks like it actually works.

I don't know the details of the python3 -m build --sdist ... command. Can we remove the various setup.py files, or are they still used?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 15, 2023 •
edited
Loading

I think the UNKNOWN-... is a symptom of not running ./bootstrap. Related:

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 15, 2023

I don't know the details of the python3 -m build --sdist ... command. Can we remove the various setup.py files, or are they still used?

No, it still goes through setuptools, so setup.py is still needed. But setuptools has deprecated its entire command-line interface (and issues deprecation warnings); going forward, it will only provide a build backend (this goes by "PEP-517"). build is a new frontend for PEP-517 builds that uses the build backend declared in pyproject.toml; in our case, setuptools.

@jhpalmieri
Copy link
Member

Re #35268, running ./bootstrap fixes the problem. This is with a brand new tarball so I think that shouldn't be necessary, which presumably is what #35268 is about.

jhpalmieri
jhpalmieri approved these changes Mar 15, 2023
View reviewed changes
Copy link
Member

@jhpalmieri jhpalmieri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, works in my limited testing.

@jhpalmieri
Copy link
Member

jhpalmieri commented Mar 15, 2023 •
edited
Loading

I wish I knew a way to list all make targets so that I could see very directly what targets were added by this change. I found https://stackoverflow.com/questions/4219255/how-do-you-get-the-list-of-targets-in-a-makefile, but for example the solution there by Marc.2377 only lists some of the targets, I'm guessing those created by the top-level Makefile.

@jhpalmieri
Copy link
Member

jhpalmieri commented Mar 15, 2023 •
edited
Loading

I have a clunky way (make -pn and searching the output) of listing the make targets, and I can see lots of new ones that maybe shouldn't be there, like texlive-sdist. Would it be easy to only create those targets for the packages for which build/pkgs/SPKG/spkg-src exists?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 15, 2023

Would it be easy to only create those targets for the packages for which build/pkgs/SPKG/spkg-src exists?

In our system, that would add more code in ./bootstrap and in m4/ and a new macro in build/make/Makefile.in; I think we shouldn't do that.

Overall, I don't think we should think of the targets of make as public API. Some certainly are, and we should probably document them (see for example #35276). But most are just implementation details.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 16, 2023

Thanks for reviewing!

@vbraun vbraun merged commit df5d71a into sagemath:develop Mar 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhpalmieri jhpalmieri jhpalmieri approved these changes

@dimpase dimpase Awaiting requested review from dimpase

Assignees

@mkoeppe mkoeppe

Labels
c: build
Projects
None yet
Milestone
sage-10.0
Development

Successfully merging this pull request may close these issues.

Remove direct use of "setup.py sdist", add targets "make SPKG-sdist"
4 participants
@mkoeppe @codecov-commenter @jhpalmieri @vbraun